Improve the config ajustment script of TLS for ENTROPY_NV_SEED #7877
Conversation
|
@andcor02 - please review. |
|
How is it going? |
|
@k-stachowiak Can you review or someone from your team? |
|
Should this actually be done to the upstream MbedTLS repo, rather than Mbed OS? MbedTLS releases are done from the MbedTLS repo. |
|
This is changing code that's automatically generated by the Mbed TLS import script which you can see here. You need to change the import script not the file itself. cc: @RonEld, @dreemkiller |
TomoYamanaka
force-pushed
the
improve_nv_seed_of_tls
branch
from
8f4f53b
to
89fc0b1
Compare
Although "nv_seed" is one of "entropy", it doesn't included to the "!defined" lineup in the following config file. Therefore, when MBEDTLS_ENTROPY_NV_SEED is defined, it is accidently invoked "mbedtls/config-no-entropy.h". mbed-os\features\mbedtls\inc\mbedtls\config.h I think that correct processing should go to line 47, not line 40.
TomoYamanaka
force-pushed
the
improve_nv_seed_of_tls
branch
from
89fc0b1
to
7c90a9e
Compare
TomoYamanaka
changed the title
|
@sbutcher-arm @JanneKiiskila |
| "#include MBEDTLS_USER_CONFIG_FILE\n" \ | ||
| "#endif\n" \ | ||
| "\n" \ | ||
| add_code \ |
There was a problem hiding this comment.
Code which looks identical is marked as changed. Are there some whitespace errors in here?
There was a problem hiding this comment.
I do not think there is such an error. I just added spaces to align the end of line.
There was a problem hiding this comment.
According to the following viewer that hides whitespace changes, you can check that there is not some whitespace errors.
https://github.com/ARMmbed/mbed-os/pull/7877/files?utf8=%E2%9C%93&diff=unified&w=1
Thus, this is due to the Diff settings.
There was a problem hiding this comment.
I have checked on my machine, and the diff is because the alignment of the ending \ has changed:
This is because the addition of \\\\ at line 49 to add the " !defined(MBEDTLS_ENTROPY_NV_SEED)\n" part in line 50, i believe
There was a problem hiding this comment.
@RonEld Thank you for comments, it is correct.
There was a problem hiding this comment.
Ok - the Mbed TLS coding standards specify 79 columns, but Mbed permit 120 and this is Mbed OS, so this is acceptable.
|
@TomoYamanaka - The problem with this code and this area is that it needs to work for lots of different types of targets and platforms. Can you say what targets you've tested this with? A build test is all that's needed - not necessarily to test and run it. |
|
@sbutcher-arm
My target is GR-PEACH that is Renesas Mbed board. Since it has not a hardware entropy(TRNG), we're considering that accessing to Mbed Cloud by utilizing NV_SEED entropy. |
|
Is there anything else required? |
| "#include MBEDTLS_USER_CONFIG_FILE\n" \ | ||
| "#endif\n" \ | ||
| "\n" \ | ||
| add_code \ |
There was a problem hiding this comment.
Ok - the Mbed TLS coding standards specify 79 columns, but Mbed permit 120 and this is Mbed OS, so this is acceptable.
|
This is a trivial change, so I think I can accept the level of testing, assuming it passed the Mbed Cloud Client CI. |
|
I don't think the Cloud Client test would verify that configuration per se. |
|
/morph build |
Build : SUCCESSBuild number : 3303 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2938 |
Test : SUCCESSBuild number : 3112 |
Description
I think that ENTROPY_NV_SEED is one of "entropy", but it doesn't included to the "!defined" lineup in the following config file.
mbed-os\features\mbedtls\inc\mbedtls\config.hTherefore, when
MBEDTLS_ENTROPY_NV_SEEDis defined, it is accidently invokedmbedtls/config-no-entropy.hat line 40.I think that it should go through line 47 in the case of
MBEDTLS_ENTROPY_NV_SEED.Pull request type